CFE-2986: Added getdir command to cf-net, copies directory and files by SimonThalvorsen · Pull Request #6049 · cfengine/core

SimonThalvorsen · 2026-03-02T10:02:31Z

Ticket: CFE-2986
Changelog: Title

cf-net/cf-net.c

larsewi

It would be nicer if we extended cf-net get to get both files and directories, instead of having two separate sub-commands. Please investigate how rsync does this and try to do it in a similar way here.

olehermanse · 2026-03-04T00:02:37Z

It would be nicer if we extended cf-net get to get both files and directories, instead of having two separate sub-commands. Please investigate how rsync does this and try to do it in a similar way here.

@larsewi on the other hand, cf-net was written mainly as a protocol testing tool, and the commands mirror their protocol command names. If the GET protocol command in CFEngine network protocol can only GET a file, it might be a bit confusing that cf-net get is something very different.

There is also some advantage to not being too dynamic with this stuff. If you make a CLI that forces the user to specify if they are copying a file or a folder, you can prevent some annoying / confusing situations.

We could for example make a new subcommand, copy;

$ cf-net copy --file promises.cf
Copying file 1.2.3.4:/var/cfengine/masterfiles/promises.cf -> ./promises.cf

$ cf-net copy --file services/
Error: 'services/' does not look like a file, did you mean --directory?

$ cf-net copy --file services/.
Error: 'services/.' does not look like a file, did you mean --directory?

$ cf-net copy --directory services/
Copying directory 1.2.3.4:/var/cfengine/masterfiles/services/ -> ./services/

$ cf-net copy services/
Copying directory 1.2.3.4:/var/cfengine/masterfiles/services/ -> ./services/

$ cf-net copy services
Warning: Ambiguous if 'services' is a file or a directory - consider adding --file or --directory.
Copying directory 1.2.3.4:/var/cfengine/masterfiles/services/ -> ./services/

larsewi

Please break this up a little. Maybe a recursive solution would be more elegant given that it is a recursive problem.

cf-net/cf-net.c

+    char *local_dir = NULL;
+
+    int argc = 0;
+    while (args[argc] != NULL)


larsewi

There should be a recursion depth limit to prevent a potential malicious or misconfigured server with symlink loops (or deeply nested dirs) to cause stack overflow.

We should probably stick to a maximum path size for the same reasons above.

cf-net/cf-net.c

larsewi

Make sure to check return values of functions. It's OK to do best effort. I.e., we encounter an error but continue if it's safe. But in this case we need to cache the error so that we can later exit with failure. For some errors, like if we fail to create a directory, it does not make sense to try to fetch the files that should be in that directory afterwards.

cf-net/cf-net.c

larsewi

It's starting to look good. Just a few more nitpicks. Almost there!

cf-net/cf-net.c

larsewi

It's starting to look very good

cf-net/cf-net.c

larsewi · 2026-03-18T13:39:32Z

cf-net/cf-net.c

+        }
+    }
+
+    args = &(args[optind]);


There's no check that a positional argument remains. Running cf-net getdir -o /tmp/ (no remote dir) will probably dereference NULL

You are still dereferencing args[optind] here without checking that there is anything there first

cf-net/cf-net.c

Ticket: CFE-2986 Changelog: Title Signed-off-by: Simon Halvorsen <simon.halvorsen@northern.tech>

larsewi

Getting there! Still a few things to address before this is ready to merge

larsewi · 2026-03-24T09:42:40Z

cf-net/cf-net.c

+            ret = process_dir_recursive(conn,
+                    remote_full,
+                    local_full,
+                    print_stats,
+                    (limit - 1));


Indents here (using 8 spaces) are a bit inconsistent with the rest of the code base

larsewi · 2026-03-24T09:47:29Z

cf-net/cf-net.c

+}
+
+// Helper: Recursively process directory entries
+static int process_dir_recursive(AgentConnection *conn,


A doc comment explaining the return values or a self documenting enum would be nice

larsewi · 2026-03-24T10:03:35Z

cf-net/cf-net.c

+}
+
+// Helper: Sets up local directory filepath
+bool static SetupLocalDirectoryPath(char *const base, char **local_dir)


Redundant comment. The function name already conveys this. The comment should answer things the signature can't tell you. Otherwise it should not be there. A better comment would for example explain what the base argument does. Because this is not obvious from the signature alone. There's a few more comments like this in the code. You can consider removing or changing them.

larsewi · 2026-03-24T10:04:19Z

cf-net/cf-net.c

+        char temp[PATH_MAX];
+
+        int written = snprintf(temp, sizeof(temp), "%s/%s/", *local_dir, base);
+        printf("tmp;%s\n", temp);


This looks like some left-over from debugging?

larsewi · 2026-03-24T10:07:16Z

cf-net/cf-net.c

+            return false;
+        }
+        free(*local_dir);
+        *local_dir = xstrdup(temp);


Here you could use StringFormat() instead. Or since the you are limiting the buffer to PATH_MAX you can require the caller to allocate it. This way the caller can allocate it on the stack.

larsewi · 2026-03-24T10:09:40Z

cf-net/cf-net.c

+            Log(LOG_LEVEL_ERR, "Path too long for local path: %s/%s/",
+                    *local_dir, base);
+            free(*local_dir);
+            free(base);


It very unexpected that this function free's something allocated by the elsewhere. The caller should be responsible.

larsewi · 2026-03-24T10:12:22Z

cf-net/cf-net.c

+                if (local_dir != NULL)
+                {
+                    Log(LOG_LEVEL_INFO,
+                        "Warning: multiple occurrences of -o in command, "\
+                        "only last one will be used.");
+                    free(local_dir);
+                }
+                local_dir = xstrdup(optarg);
+                break;


Maybe it would be better to just return failure here?

larsewi · 2026-03-24T10:13:35Z

cf-net/cf-net.c

+                printf("Default optarg = '%s', c = '%c' = %i\n",
+                       optarg, c, (int)c);
+                break;


Please explain

larsewi · 2026-03-24T10:15:45Z

cf-net/cf-net.c

+        }
+    }
+
+    args = &(args[optind]);


You are still dereferencing args[optind] here without checking that there is anything there first

SimonThalvorsen requested a review from larsewi March 2, 2026 10:02

github-advanced-security bot found potential problems Mar 2, 2026

View reviewed changes

cf-net/cf-net.c Fixed Show fixed Hide fixed

cf-net/cf-net.c Fixed Show fixed Hide fixed

SimonThalvorsen force-pushed the CFE-2986 branch from f7cebad to 40a6c09 Compare March 2, 2026 10:40

larsewi changed the title ~~Added getdir command to cf-net, copies directory and files~~ CFE-2986: Added getdir command to cf-net, copies directory and files Mar 2, 2026

larsewi requested changes Mar 2, 2026

View reviewed changes

SimonThalvorsen force-pushed the CFE-2986 branch from 40a6c09 to 2bd2729 Compare March 4, 2026 14:58

larsewi requested changes Mar 5, 2026

View reviewed changes

cf-net/cf-net.c Outdated Show resolved Hide resolved

cf-net/cf-net.c Outdated Show resolved Hide resolved

cf-net/cf-net.c Outdated Show resolved Hide resolved

cf-net/cf-net.c Outdated Show resolved Hide resolved

cf-net/cf-net.c Outdated Show resolved Hide resolved

SimonThalvorsen force-pushed the CFE-2986 branch from 2bd2729 to e9f4cb3 Compare March 6, 2026 13:53

github-advanced-security bot found potential problems Mar 6, 2026

View reviewed changes

cf-net/cf-net.c Fixed Show fixed Hide fixed

cf-net/cf-net.c Fixed Show fixed Hide fixed

cf-net/cf-net.c Fixed Show fixed Hide fixed

cf-net/cf-net.c Fixed Show fixed Hide fixed

SimonThalvorsen force-pushed the CFE-2986 branch from e9f4cb3 to 5b55d16 Compare March 6, 2026 14:12

github-advanced-security bot found potential problems Mar 6, 2026

View reviewed changes

cf-net/cf-net.c

char *local_dir = NULL;

int argc = 0;

while (args[argc] != NULL)

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 115 lines.

SimonThalvorsen requested a review from larsewi March 9, 2026 14:42

larsewi requested changes Mar 10, 2026

View reviewed changes

SimonThalvorsen force-pushed the CFE-2986 branch 2 times, most recently from d68100f to 047b068 Compare March 12, 2026 14:29

SimonThalvorsen requested a review from larsewi March 12, 2026 14:37

larsewi requested changes Mar 13, 2026

View reviewed changes

SimonThalvorsen force-pushed the CFE-2986 branch 2 times, most recently from 62eb2df to c2b59c7 Compare March 13, 2026 15:49

larsewi requested changes Mar 16, 2026

View reviewed changes

SimonThalvorsen force-pushed the CFE-2986 branch from c2b59c7 to 38becec Compare March 16, 2026 13:54

SimonThalvorsen requested a review from larsewi March 16, 2026 13:56

github-advanced-security bot found potential problems Mar 16, 2026

View reviewed changes

cf-net/cf-net.c Fixed Show fixed Hide fixed

SimonThalvorsen force-pushed the CFE-2986 branch from 38becec to 1b062de Compare March 16, 2026 14:21

larsewi requested changes Mar 18, 2026

View reviewed changes

Added getdir command to cf-net, recursively copies directory

04e28f6

Ticket: CFE-2986 Changelog: Title Signed-off-by: Simon Halvorsen <simon.halvorsen@northern.tech>

SimonThalvorsen force-pushed the CFE-2986 branch from 1b062de to 04e28f6 Compare March 20, 2026 13:14

SimonThalvorsen requested a review from larsewi March 23, 2026 14:09

larsewi requested changes Mar 24, 2026

View reviewed changes

Conversation

SimonThalvorsen commented Mar 2, 2026

Uh oh!

Uh oh!

Uh oh!

larsewi left a comment

Choose a reason for hiding this comment

Uh oh!

olehermanse commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsewi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

larsewi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larsewi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larsewi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larsewi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larsewi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

olehermanse commented Mar 4, 2026 •

edited

Loading